Skip to content

Fix safety doc in intrinsics::simd#159322

Open
yilin0518 wants to merge 1 commit into
rust-lang:mainfrom
yilin0518:fix_simd_1
Open

Fix safety doc in intrinsics::simd#159322
yilin0518 wants to merge 1 commit into
rust-lang:mainfrom
yilin0518:fix_simd_1

Conversation

@yilin0518

@yilin0518 yilin0518 commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

The Safety documentation for simd_gather and simd_scatter incorrectly described unmasked values in T as requiring readable or writable memory.

The memory access is performed through the corresponding enabled pointer lane in ptr, so this updates the documentation to state that requirement explicitly.

@rustbot

rustbot commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

Some changes occurred to the platform-builtins intrinsics. Make sure the
LLVM backend as well as portable-simd gets adapted for the changes.

cc @antoyo, @GuillaumeGomez, @bjorn3, @calebzulawski, @programmerjake

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 15, 2026
@rustbot

rustbot commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

r? @JohnTitor

rustbot has assigned @JohnTitor.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: libs
  • libs expanded to 12 candidates
  • Random selection from 6 candidates

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@RalfJung

RalfJung commented Jul 15, 2026 via email

Copy link
Copy Markdown
Member

@RalfJung

RalfJung commented Jul 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Looking at the diff, I think the old and new wording are equivalent. An "unmasked value" is likely meant to be interpreted as a value in a lane that has not been "masked out", i.e. in an enabled lane. But I agree that the old wording is confusing.

So r=me on the change, but CI failed because the doc link is not right.
@rustbot author
r? me

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 15, 2026
@rustbot

rustbot commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@rustbot rustbot assigned RalfJung and unassigned JohnTitor Jul 15, 2026
@yilin0518

yilin0518 commented Jul 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Is there LLVM documentation confirming the exact requirements here?

I found the possible corresponding documentation: llvm-masked-gather-intrinsics and llvm-masked-scatter-intrinsics, in which it emphasizes that ptr should be readable/writable if the value in mask is !0.

@yilin0518

yilin0518 commented Jul 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@RalfJung I commit the fixed commit, it should have solved the link problem.

Besides, I open a topic in Zulip: Maybe some safety doc need fixs in intrinsics::simd. In this topic I refered another functions: simd_masked_load and simd_masked_store. I think the safety section of these function should add an extra entry: Enabled ptr elements must be safe to read/write as if by core::ptr::read/write. Can you provide any suggestion? Thank you!

Also, I think simd_funnel_shl, simd_funnel_shr, simd_shl, simd_shr need to be modified: change less than <int>::BITS to in 0..<int>::BITS. If really need, I can provide a new PR to fix these functions.

RalfJung
RalfJung reviewed Jul 15, 2026
View reviewed changes
Comment thread library/core/src/intrinsics/simd/mod.rs Outdated
/// Unmasked values in `T` must be readable as if by `<ptr>::read` (e.g. aligned to the element
/// type).
/// Each pointer in `ptr` whose corresponding value in `mask` is `!0` must be readable as if by
/// [`crate::ptr::read`] (e.g. aligned to the element type).

@RalfJung RalfJung Jul 15, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now the crate:: will show up in the rendered text, that's not great.
Something like [`ptr::read`][crate::ptr::read] should work.

View changes since the review

@RalfJung RalfJung Jul 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You dud not copy what I suggested and did not fix the problem I think.

@yilin0518 yilin0518 Jul 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, thank you for your remind. I think I do the right thing this time.😭

@RalfJung

RalfJung commented Jul 15, 2026

Copy link
Copy Markdown
Member

Also note that this is still marked as waiting-for-author. As the bot says, please say @rustbot ready if you want another review. Sometimes reviewers will guess that the PR is ready for review but that doesn't always work.

@yilin0518

yilin0518 commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 15, 2026
@RalfJung

RalfJung commented Jul 15, 2026

Copy link
Copy Markdown
Member

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 15, 2026
@yilin0518

yilin0518 commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 15, 2026
@RalfJung

RalfJung commented Jul 16, 2026

Copy link
Copy Markdown
Member

Thanks :)
@bors r+ rollup

@rust-bors

rust-bors Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

📌 Commit 95ae48e has been approved by RalfJung

It is now in the queue for this repository.

@rust-bors rust-bors Bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 16, 2026
JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Jul 16, 2026
@JonathanBrouwer
Rollup merge of rust-lang#159322 - yilin0518:fix_simd_1, r=RalfJung
6578cac 
Fix safety doc in intrinsics::simd

The Safety documentation for `simd_gather` and `simd_scatter` incorrectly described unmasked values in `T` as requiring readable or writable memory.

The memory access is performed through the corresponding enabled pointer lane in `ptr`, so this updates the documentation to state that requirement explicitly.
@JonathanBrouwer JonathanBrouwer mentioned this pull request Jul 16, 2026
Open
rust-bors Bot pushed a commit that referenced this pull request Jul 16, 2026
@bors
Auto merge of #159407 - JonathanBrouwer:rollup-Glp5Zkf, r=JonathanBro…
3d50c25 
…uwer

Rollup of 18 pull requests

Successful merges:

 - #150679 (dirfd file operations (2/4))
 - #158758 (Rename `lazy_type_alias` feature gate to `checked_type_aliases` )
 - #159177 (Explicitly materialize debuginfo tests for all debuggers)
 - #159392 (Update Rust crate rand to v0.9.3 [SECURITY])
 - #159393 (Update Rust crate rkyv to v0.8.16 [SECURITY])
 - #159394 (Update Rust crate tar to v0.4.46 [SECURITY])
 - #159395 (Update Rust crate tracing-subscriber [SECURITY])
 - #159193 (Port compiletest's CLI to clap)
 - #158992 (Shorten types more when only interesting part is lifetimes)
 - #159226 (Track extra_lifetime_params_map per-owner)
 - #159273 (tidy: document u64 limit for numeric sort keys)
 - #159322 (Fix safety doc in intrinsics::simd)
 - #159329 (Fix the stale metrics directory warning)
 - #159357 (Add regression tests for two fixed issues)
 - #159360 (Remove FIXMEs referencing #44232)
 - #159375 (make a couple codegen-llvm tests compatible with 2021 edition)
 - #159381 (emit `(lldb)` prefix to make commands more distinct)
 - #159396 (Update actions/checkout action to v7)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RalfJung RalfJung RalfJung left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@RalfJung RalfJung

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@yilin0518 @rustbot @rust-log-analyzer @RalfJung @JohnTitor